fix(cw-context): encoding, decoding and iteration of `ConsensusState` heights #1176

rnbguy · 2024-04-19T14:05:53Z

Closes: #1175

This PR also replaces string formatting and parsing with protobuf encoding and decoding.

Description

PR author checklist:

Added changelog entry, using unclog.
Added tests.
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).
Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

codecov · 2024-04-19T14:13:27Z

Codecov Report

Attention: Patch coverage is 51.19048% with 41 lines in your changes are missing coverage. Please review.

Project coverage is 64.35%. Comparing base (d5e3887) to head (6dc68cf).
Report is 1 commits behind head on main.

Files	Patch %	Lines
ibc-clients/cw-context/src/context/mod.rs	49.29%	36 Missing ⚠️
ibc-clients/cw-context/src/context/client_ctx.rs	61.53%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1176      +/-   ##
==========================================
- Coverage   64.39%   64.35%   -0.04%     
==========================================
  Files         230      229       -1     
  Lines       22053    22081      +28     
==========================================
+ Hits        14201    14211      +10     
- Misses       7852     7870      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

romac · 2024-04-19T14:52:11Z

Looks like this fixed CI on informalsystems/hermes#3943, not seeing the error anymore even with super short trusting period 🚀

ibc-clients/cw-context/src/utils/mod.rs

ibc-clients/cw-context/src/context/mod.rs

Farhad-Shabani · 2024-04-19T21:27:17Z

.changelog/unreleased/bug-fixes/1175-cw-consensus-state-height-iteration-codec.md

+- [ibc-clients/cw-context] Fix `ConsensusState` height iteration.
+  ([\#1175](https://github.com/cosmos/ibc-rs/issues/1175))
+- [ibc-clients/cw-context] Encode and decode `ConsensusState` heights using
+  Protobuf. ([\#1176](https://github.com/cosmos/ibc-rs/pull/1176))


As this crate has recently been added (Not present in any versions yet), not sure if we should reflect this fix in the changelog.

Ah I didn't know this convention. Removed.

7a10782

Farhad-Shabani

Thanks for handling this, @rnbguy! I left a few comments. It would be great if we could reproduce the issue with a unit test, but let's put that off for now. Otherwise, everything looks good.

rnbguy · 2024-04-20T10:35:29Z

ibc-clients/cw-context/src/context/mod.rs

+pub const CONSENSUS_STATE_HEIGHT_MAP: Map<'_, (u64, u64), Empty> =
+    Map::new(ITERATE_CONSENSUS_STATE_PREFIX);


I started using this for a better interface. I am curious - if anything breaks if I do this.

Cool. We should be good as long as the key produced by the Map matches the iteration_key() (?)
Specifically, we only need to use the iteration_key here. (Connected to your question below)

I realized in the old implementation, we should have used this range_with_prefix from cw-storage-plus. Note the trick for None and exclusive or inclusive cases in calc_start_bound and calc_end_bound.

Since this brings cw-storage-plus dependency anyway, it's best to use their storage abstractions, built on top of these low-level functions.

rnbguy · 2024-04-20T10:36:59Z

ibc-clients/cw-context/src/context/mod.rs

+        for (rev_number, rev_height) in iterator {
+            let height = Height::new(rev_number, rev_height)?;

-        for (key, height) in iterator {
-            metadata.push(GenesisMetadata { key, value: height });
+            metadata.push(GenesisMetadata {
+                key: iteration_key(rev_number, rev_height),
+                value: height.encode_vec(),
+            });


What is the purpose of passing Height here?

As for the why, haven’t had a chance yet to explore where/how this GenesisMetaData gonna be used. But the implementation mirrors ibc-go.

rnbguy · 2024-04-21T08:48:55Z

I will merge this after confirming the latest changes on the Hermes PR 🎉

rnbguy · 2024-04-22T10:20:43Z

Tested on personal fork of informalsystems/hermes using workflow artifact from latest commit. Merging it 🚀

Note: once hermes is released with wasm client support. basecoin-rs should have an e2e test for wasm clients.

… heights (#1176) * only consider ITERATE_CONSENSUS_STATE_PREFIX keys * use proto codec * refactor with updated parse_height type sig * encode and decode height funcs * fix clippy * rm redundant type annotation * add changelog entry * correct link on changelog * rm changelog entry * handle missing case * use cw_storage_plus::Map * store or delete height keys at cw_storage_plus::Map * use cw_storage_plus::Map range or keys * rm redundant encode_height and decode_height * add comment * use imports * cargo format * deserialized result must be handled * update doc string

rnbguy added 5 commits April 19, 2024 15:52

only consider ITERATE_CONSENSUS_STATE_PREFIX keys

5e1ec84

use proto codec

db93358

refactor with updated parse_height type sig

8aa653b

encode and decode height funcs

b88734b

fix clippy

ebfbb7b

rnbguy marked this pull request as ready for review April 19, 2024 14:13

rnbguy requested a review from romac April 19, 2024 14:13

rnbguy changed the title ~~fix(wasm08): ConsensusState height iteration, encoding and decoding~~ fix(wasm08): encoding, decoding and iteration of ConsensusState heights Apr 19, 2024

rm redundant type annotation

4de6db9

rnbguy changed the title ~~fix(wasm08): encoding, decoding and iteration of ConsensusState heights~~ fix(cw-context): encoding, decoding and iteration of ConsensusState heights Apr 19, 2024

rnbguy requested a review from Farhad-Shabani April 19, 2024 16:28

rnbguy added 2 commits April 19, 2024 18:33

add changelog entry

f5bc322

correct link on changelog

94683f4

Farhad-Shabani reviewed Apr 19, 2024

View reviewed changes

ibc-clients/cw-context/src/utils/mod.rs Outdated Show resolved Hide resolved

Farhad-Shabani reviewed Apr 19, 2024

View reviewed changes

ibc-clients/cw-context/src/context/mod.rs Show resolved Hide resolved

Farhad-Shabani reviewed Apr 19, 2024

View reviewed changes

Farhad-Shabani approved these changes Apr 19, 2024

View reviewed changes

Farhad-Shabani added this to the 0.52.0 milestone Apr 19, 2024

rnbguy added 6 commits April 20, 2024 11:40

rm changelog entry

7a10782

handle missing case

5133dfb

use cw_storage_plus::Map

9c76cc8

store or delete height keys at cw_storage_plus::Map

edcaab3

use cw_storage_plus::Map range or keys

c6331d5

rm redundant encode_height and decode_height

7aa0c57

rnbguy commented Apr 20, 2024

View reviewed changes

rnbguy added 2 commits April 20, 2024 12:44

add comment

b442f93

use imports

41bed59

rnbguy added 3 commits April 20, 2024 12:47

cargo format

4a4679c

deserialized result must be handled

b082200

update doc string

6dc68cf

rnbguy force-pushed the rano/fix/wasm08-height-iteration branch from fd7b3db to 6dc68cf Compare April 21, 2024 08:47

Farhad-Shabani mentioned this pull request Apr 22, 2024

imp: ehnance ICS-07 CosmWasm tests using Timestamp::now() + add test_cw_client_expiry #1181

Merged

4 tasks

rnbguy added this pull request to the merge queue Apr 22, 2024

Merged via the queue into main with commit d0f7cc4 Apr 22, 2024
20 checks passed

rnbguy deleted the rano/fix/wasm08-height-iteration branch April 22, 2024 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cw-context): encoding, decoding and iteration of `ConsensusState` heights #1176

fix(cw-context): encoding, decoding and iteration of `ConsensusState` heights #1176

rnbguy commented Apr 19, 2024 •

edited

Loading

codecov bot commented Apr 19, 2024 •

edited

Loading

romac commented Apr 19, 2024

Farhad-Shabani Apr 19, 2024

rnbguy Apr 20, 2024

Farhad-Shabani left a comment

rnbguy Apr 20, 2024

Farhad-Shabani Apr 21, 2024

rnbguy Apr 21, 2024

rnbguy Apr 20, 2024

Farhad-Shabani Apr 21, 2024

rnbguy commented Apr 21, 2024

rnbguy commented Apr 22, 2024 •

edited

Loading

		pub const CONSENSUS_STATE_HEIGHT_MAP: Map<'_, (u64, u64), Empty> =
		Map::new(ITERATE_CONSENSUS_STATE_PREFIX);

fix(cw-context): encoding, decoding and iteration of ConsensusState heights #1176

fix(cw-context): encoding, decoding and iteration of ConsensusState heights #1176

Conversation

rnbguy commented Apr 19, 2024 • edited Loading

Description

PR author checklist:

Reviewer checklist:

codecov bot commented Apr 19, 2024 • edited Loading

Codecov Report

romac commented Apr 19, 2024

Farhad-Shabani Apr 19, 2024

Choose a reason for hiding this comment

rnbguy Apr 20, 2024

Choose a reason for hiding this comment

Farhad-Shabani left a comment

Choose a reason for hiding this comment

rnbguy Apr 20, 2024

Choose a reason for hiding this comment

Farhad-Shabani Apr 21, 2024

Choose a reason for hiding this comment

rnbguy Apr 21, 2024

Choose a reason for hiding this comment

rnbguy Apr 20, 2024

Choose a reason for hiding this comment

Farhad-Shabani Apr 21, 2024

Choose a reason for hiding this comment

rnbguy commented Apr 21, 2024

rnbguy commented Apr 22, 2024 • edited Loading

fix(cw-context): encoding, decoding and iteration of `ConsensusState` heights #1176

fix(cw-context): encoding, decoding and iteration of `ConsensusState` heights #1176

rnbguy commented Apr 19, 2024 •

edited

Loading

codecov bot commented Apr 19, 2024 •

edited

Loading

rnbguy commented Apr 22, 2024 •

edited

Loading